fix: docker availability precheck#596
Conversation
🦋 Changeset detectedLatest commit: 2afa875 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CKodidela
changed the title
|
coverage: 64.85% (-0.3%) from 65.131% — CKodidela:fix/docker-availability-precheck into cameri:main |
|
@cameri up for review |
There was a problem hiding this comment.
Pull request overview
This PR normalizes runCommandWithOutput so CLI commands receive a discriminated CommandResult instead of handling spawn-time rejections ad hoc. In the CLI codebase, that mainly addresses Docker/Git availability failures more consistently and adds regression coverage for the info --json Docker-missing case.
Changes:
- Added a
CommandResultunion torunCommandWithOutputand converted spawn/timeout/signal failures into resolved results. - Updated
infoandupdatecommand paths to handle the new result shape. - Added/updated CLI tests for the new contract, including a regression test for Docker missing in
info --json.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/cli/utils/process.ts |
Changes the command runner contract from reject-on-spawn-error to resolved union results. |
src/cli/commands/info.ts |
Updates relay uptime lookup to handle CommandResult.ok. |
src/cli/commands/update.ts |
Adds handling for git stash spawn failures during update flow. |
test/unit/cli/run-command.spec.ts |
Adds direct tests for the new runCommandWithOutput result variants. |
test/unit/cli/info.spec.ts |
Updates stubs to new shape and adds Docker-missing JSON regression coverage. |
test/unit/cli/update.spec.ts |
Updates existing stubs to the new CommandResult shape. |
.changeset/normalize-run-command-with-output.md |
Records the patch release note for the CLI runner contract change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| options: RunOptions = {}, | ||
| ): Promise<{ code: number; stdout: string; stderr: string }> => { | ||
| return new Promise((resolve, reject) => { | ||
| ): Promise<CommandResult> => { |
| spinner.fail(stashResult.ok === false && stashResult.reason === 'not-found' ? 'Update failed: git is not installed' : 'Update failed while stashing local changes') | ||
| return 1 | ||
| } | ||
| if (stashResult.code !== 0) { | ||
| spinner.fail('Update failed while stashing local changes') |
| if (!stashResult.ok) { | ||
| spinner.fail(stashResult.ok === false && stashResult.reason === 'not-found' ? 'Update failed: git is not installed' : 'Update failed while stashing local changes') | ||
| return 1 |
|
will resolve this soon |
Description
Replaces the per-call-site
try/catchingetRelayUptimeSecondswith a normalized result type onrunCommandWithOutput. Thefunction now always resolves with a
CommandResultdiscriminated union instead of rejecting on spawn errors, giving every callera consistent contract.
Related Issue
Closes #590
Motivation and Context
spawnemits anENOENTerror (not a non-zero exit code) when the target command is not installed. Before this change,runCommandWithOutputforwarded that rejection to callers, which causedinfo --jsonto crash when Docker was absent. Theshort-term fix was a
try/catchat the call site, but the same class of bug could recur in any other wrapper.The fix moves the concern into the shared runner so all callers benefit automatically.
How Has This Been Tested?
outputs valid JSON when docker is not installed (ENOENT)) that stubsrunCommandWithOutputtoreturn
{ ok: false, reason: 'not-found' }and assertsinfo --jsonstill exits 0 with valid JSON.info.spec.tsandupdate.spec.tsto use the newCommandResultshape.npm run test:cli).Types of changes
Checklist: